[orchestrator-v2] fix(orchestrator): Prevent Claude session release from hanging on idle CLI reads#3756
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| const idAllocator = yield* IdAllocatorV2; | ||
| const queryRunner = yield* ClaudeAgentSdkQueryRunner; | ||
| const serverConfig = yield* ServerConfig; | ||
| const continuationRequests = yield* ProviderContinuationRequests; |
There was a problem hiding this comment.
🟠 High Adapters/ClaudeAdapterV2.ts:3829
ClaudeAdapterV2Driver.create reads ProviderContinuationRequests from the context, but this is a Context.Reference whose default drops all offers. In production, providerAdapterRegistryLayerFromProviderInstances does not provide providerContinuationRequestsLayer, so the driver receives the default sink. Every Claude adapter then stores offer: () => Effect.void, and all background-task wake turns are silently dropped instead of reaching ProviderContinuationService — the continuation mechanism never runs outside tests. Make the production registry layer provide providerContinuationRequestsLayer, or change the driver to require a non-default implementation so the missing provider fails loudly instead of silently dropping work.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts around line 3829:
`ClaudeAdapterV2Driver.create` reads `ProviderContinuationRequests` from the context, but this is a `Context.Reference` whose default drops all offers. In production, `providerAdapterRegistryLayerFromProviderInstances` does not provide `providerContinuationRequestsLayer`, so the driver receives the default sink. Every Claude adapter then stores `offer: () => Effect.void`, and all background-task wake turns are silently dropped instead of reaching `ProviderContinuationService` — the continuation mechanism never runs outside tests. Make the production registry layer provide `providerContinuationRequestsLayer`, or change the driver to require a non-default implementation so the missing provider fails loudly instead of silently dropping work.
f0bd0f5 to
e94b5f4
Compare
There was a problem hiding this comment.
🟡 Medium
releaseIfStillIdle reuses the stale pinnedSinceMs when re-arming the idle timer after deferring for pending background work. Activity paths that go through touchActivity or markIdle (e.g. respondToRuntimeRequest, steerTurn, interruptTurn, or provider events) never reset pinnedSinceMs — only markBusy clears it. So a session that had pending work, then later receives fresh activity without ever becoming busy, will still carry the original pin timestamp. The next idle check sees now - pinnedSinceMs >= maxIdlePinMs and immediately releases the session, even though it had recent activity and still reports pending background work. Resetting pinnedSinceMs to null in touchActivity would clear the stale pin on fresh activity.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderSessionManager.ts around line 712:
`releaseIfStillIdle` reuses the stale `pinnedSinceMs` when re-arming the idle timer after deferring for pending background work. Activity paths that go through `touchActivity` or `markIdle` (e.g. `respondToRuntimeRequest`, `steerTurn`, `interruptTurn`, or provider events) never reset `pinnedSinceMs` — only `markBusy` clears it. So a session that had pending work, then later receives fresh activity without ever becoming `busy`, will still carry the original pin timestamp. The next idle check sees `now - pinnedSinceMs >= maxIdlePinMs` and immediately releases the session, even though it had recent activity and still reports pending background work. Resetting `pinnedSinceMs` to `null` in `touchActivity` would clear the stale pin on fresh activity.
| }, | ||
| ); | ||
|
|
||
| yield* requests.take.pipe( |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/ProviderContinuationService.ts:56
The worker loop permanently drops a continuation request whenever dispatchContinuation fails. requests.take removes the item from the queue, but the Effect.catchCause handler only logs the failure and never retries or re-enqueues it — so the buffered provider wake event is lost and never ingested as a run. This is reachable via the queue_after_active dispatch path, which rejects dispatch while a thread has pending merge-back transfers. Consider re-enqueuing the request (or retrying with backoff) before logging so the wake event survives a transient rejection.
Also found in 1 other location(s)
apps/server/src/orchestration-v2/ProviderSessionManager.ts:44
Using the new
RELEASE_SCOPE_CLOSE_TIMEOUT_MScausesreleaseEntry/close()to report success after 30s even whenScope.close(entry.scope, Exit.void)later fails. OnceEffect.timeoutOption(...)returnsNone, the code only logs the eventualExitin a detached observer and never re-raises it asProviderSessionReleaseError/ProviderSessionCloseError, so callers cannot detect adapter cleanup failures anymore on any close path that finishes after the timeout.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderContinuationService.ts around line 56:
The worker loop permanently drops a continuation request whenever `dispatchContinuation` fails. `requests.take` removes the item from the queue, but the `Effect.catchCause` handler only logs the failure and never retries or re-enqueues it — so the buffered provider wake event is lost and never ingested as a run. This is reachable via the `queue_after_active` dispatch path, which rejects dispatch while a thread has pending merge-back transfers. Consider re-enqueuing the request (or retrying with backoff) before logging so the wake event survives a transient rejection.
Also found in 1 other location(s):
- apps/server/src/orchestration-v2/ProviderSessionManager.ts:44 -- Using the new `RELEASE_SCOPE_CLOSE_TIMEOUT_MS` causes `releaseEntry`/`close()` to report success after 30s even when `Scope.close(entry.scope, Exit.void)` later fails. Once `Effect.timeoutOption(...)` returns `None`, the code only logs the eventual `Exit` in a detached observer and never re-raises it as `ProviderSessionReleaseError`/`ProviderSessionCloseError`, so callers cannot detect adapter cleanup failures anymore on any close path that finishes after the timeout.
6c7e27e to
90e67c3
Compare
| ordinal: projection.messages.length + 1, | ||
| }); | ||
| const commandId = CommandId.make(`provider-continuation:${messageId}`); | ||
| yield* threads.dispatch({ |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/ProviderContinuationService.ts:42
dispatchContinuation discards the providerThreadId and driver from the ProviderContinuationRequest, dispatching a generic message.dispatch against the thread's current routing. If the thread's active provider changes before the worker drains the request (e.g. after a provider switch or handoff), the synthetic wake turn starts on the wrong provider thread while the buffered provider-native wake messages remain attached to the original provider thread and are never ingested. Consider passing request.providerThreadId and request.driver through to threads.dispatch so the continuation targets the correct provider.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderContinuationService.ts around line 42:
`dispatchContinuation` discards the `providerThreadId` and `driver` from the `ProviderContinuationRequest`, dispatching a generic `message.dispatch` against the thread's current routing. If the thread's active provider changes before the worker drains the request (e.g. after a provider switch or handoff), the synthetic wake turn starts on the wrong provider thread while the buffered provider-native wake messages remain attached to the original provider thread and are never ingested. Consider passing `request.providerThreadId` and `request.driver` through to `threads.dispatch` so the continuation targets the correct provider.
There was a problem hiding this comment.
🟡 Medium
When a session has pending background work, releaseIfStillIdle calls scheduleIdleReleaseInternal, which immediately calls cancelIdleFiber(entry.idleFiber). Because entry.idleFiber still points to the currently running idle-release fiber, the fiber interrupts itself and then joins its own interruption, deadlocking forever. Any session that defers idle release for pending background work will wedge instead of rescheduling. Consider passing cancelIdleFiber: false to the reschedule path (or skipping the cancel when the current fiber is the idle fiber) so the self-interrupt does not happen.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderSessionManager.ts around line 673:
When a session has pending background work, `releaseIfStillIdle` calls `scheduleIdleReleaseInternal`, which immediately calls `cancelIdleFiber(entry.idleFiber)`. Because `entry.idleFiber` still points to the currently running idle-release fiber, the fiber interrupts itself and then joins its own interruption, deadlocking forever. Any session that defers idle release for pending background work will wedge instead of rescheduling. Consider passing `cancelIdleFiber: false` to the reschedule path (or skipping the cancel when the current fiber is the idle fiber) so the self-interrupt does not happen.
| providerSessionId: input.providerSessionId, | ||
| reason: "idle_timeout", | ||
| cancelIdleFiber: false, | ||
| onlyIfIdleGeneration: input.generation, |
There was a problem hiding this comment.
Idle release stale pending check
High Severity
releaseIfStillIdle decides not to defer when hasPendingBackgroundWork is false, then calls releaseEntry without re-checking pending work. Wake output can land in the adapter buffer after that read but before the session is torn down, so idle release can drop a live Claude session while wake messages and a queued continuation are still in memory.
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
| ? false | ||
| : yield* entry.runtime.hasPendingBackgroundWork.pipe( | ||
| Effect.catchCause(() => Effect.succeed(false)), | ||
| ); |
There was a problem hiding this comment.
Pending work errors imply none
Medium Severity
When hasPendingBackgroundWork fails, releaseIfStillIdle treats the session as having no pending work via Effect.catchCause(() => Effect.succeed(false)), so idle release can proceed while wake buffers or background tasks may still be active.
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
| providerThreadId: route.providerThreadId, | ||
| driver: CLAUDE_PROVIDER, | ||
| detail, | ||
| }); |
There was a problem hiding this comment.
Failed continuation blocks retry
Medium Severity
When a wake is detected, the Claude adapter records the native thread in requestedContinuations before enqueueing a continuation request. If ProviderContinuationService later skips or fails dispatch (for example archived thread, orchestration error, or logged dispatch-failed), nothing clears that flag or sends another request. Further wake output stays buffered, hasPendingBackgroundWork remains true, and the wake may never run until idle pin expiry or session release.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. This PR introduces significant new capability (background task wake handling, session pinning, continuation service) beyond a simple fix. Multiple unresolved review comments identify high-severity issues including production misconfiguration causing silent drops and potential deadlocks in the idle release path. You can customize Macroscope's approvability policy. Learn more. |
90e67c3 to
fc4490c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
There are 6 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fc4490c. Configure here.
| cause, | ||
| }), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Dropped continuation blocks wake retry
Medium Severity
When the continuation worker skips or logs-and-swallows a failed message.dispatch (for example an archived thread), the Claude adapter still keeps requestedContinuations set and wake data buffered. No second continuation is offered, so wake output may never be ingested until idle pin expiry tears down the session.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fc4490c. Configure here.
| dispatchMode: { type: "queue_after_active" }, | ||
| createdBy: "agent", | ||
| creationSource: "provider", | ||
| }); |
There was a problem hiding this comment.
Continuation ignores wake provider thread
Medium Severity
Wake continuation requests carry providerThreadId, but dispatch only passes threadId into message.dispatch, so the orchestrator binds the run to activeProviderThreadId. If that differs from the thread that owns the buffered wake session, the continuation run opens the wrong provider session and the wake buffer is never drained.
Reviewed by Cursor Bugbot for commit fc4490c. Configure here.
| providerThreadId: route.providerThreadId, | ||
| driver: CLAUDE_PROVIDER, | ||
| detail, | ||
| }); |
There was a problem hiding this comment.
Stale background task registry
Medium Severity
pendingBackgroundTaskIds is only cleared when a task_notification is handled on an active turn. Wake output buffered outside a turn (including a terminal result without a replayed notification) never removes those IDs, so hasPendingBackgroundWork can stay true after the wake is ingested and idle release is deferred until the pin cap on every idle cycle.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fc4490c. Configure here.
Introduce a provider-agnostic continuation request queue, worker, and optional hasPendingBackgroundWork idle pin so adapters can surface post-settle native wake traffic as first-class runs. No adapter offers continuations yet; behavior is unchanged until Claude or Grok specialty commits wire attach mode.
…on runs When a background task (run_in_background Bash) settles, the Claude CLI streams a whole new turn over the still-open SDK query. Buffer null-activeTurn wake messages, request one internal continuation run per wake, and drain the buffer in attach mode without re-prompting the CLI. Report pending background work so idle release stays pinned until the wake is ingested.
fc4490c to
84245db
Compare


Stacked on #3752 (which stacks on #3750); the last commit is this PR's delta.
Summary
Releasing an idle Claude provider session could deadlock silently after
removing the in-memory entry: the CLI child process stayed alive
indefinitely, no released event was persisted, the thread projection's
provider session stayed
"ready", and nothing was logged. The stale CLIkeeps holding the native session file, which is the likely original
mechanism behind
Session ID ... is already in use(#3750 works around thatsymptom by forcing
--resume; this PR fixes the leak itself).Discovered while live-verifying #3750/#3752 against a real 30-minute idle
release: the release fired (the entry was deleted), then wedged before
query.close, leaving the CLI orphaned.Problem and Fix
openQuery) is interrupted before thecloseSessionfinalizer that callsquery.close. Interruption runs Effect'sChannel.fromAsyncIterablefinalizerEffect.promise(() => iter.return()), anditeris the SDKQuery's rawsdkMessagesasync generator (that is whatQuery[Symbol.asyncIterator]()returns).return()on a generator suspended at an internalawait(reading the next message from an idle CLI) queues behind the in-flight read forever, so the scope close never completes andquery.closeis never reached.claudeQueryMessageshands the stream an iterable whose[Symbol.asyncIterator]()returns theQueryitself. The SDK'sQuery.return()runscleanup()first, which closes the transport, terminates the CLI child, and unblocks the pending read, so interruption completes and the remaining finalizers run.Query.next()already delegates tosdkMessages.next(), so streaming behavior is unchanged.Defensive Fixes
releaseEntryforever with no released events and no log line, leaving the projection stuck on"ready".releaseEntryforks the scope close (Effect.forkDetach({ startImmediately: true })) and joins with a 30s timeout. On timeout it logsprovider-session-scope-close-timeout, attaches a detached observer that logs late completion or failure, and persists the released events regardless.startImmediatelykeeps well-behaved synchronous finalizer stacks completing on the same tick, and close failures still propagate after event writes exactly as before.Known limitations
if a close still wedges (a future adapter regression), the projection
shows "stopped" while the provider process may linger; the
provider-session-scope-close-timeoutwarning is the operational signal.Close failures that finish after the timeout are logged by the detached
observer rather than surfaced as
ProviderSessionReleaseError.step revokes MCP credentials thread-wide, so a replacement provider
session for the same thread opened during the close window could have its
fresh credentials revoked when the old release finishes. The window
already existed (entry removal precedes close and MCP cleanup); per-entry
credential ownership is a candidate follow-up. With the root-cause fix in
place the Claude close path no longer wedges, so hitting the widened
window requires a new bug.
Validation
vp check: 0 errors;vp run typecheck: exit 0 (full monorepo)ClaudeAdapterV2.test.ts: 20/20, including a new regression test thatinterrupts the message stream while a read is in flight; with the wrapper
reverted to the raw generator the test deadlocks and times out at 60s
(verified during development)
ProviderSessionManager.test.ts: 19/19, including a new test where ahanging session-scope finalizer no longer blocks release: released events
persist, and the wedged finalizer is confirmed via the adapter close count
staying 0
ClaudeReplayFixtures.integration.test.ts: 4 pass / 1 skipped;OrchestratorReplayFixtures.integration.test.ts -t "claude": 16 pass /53 skipped
Note
High Risk
Touches provider session lifecycle, Claude adapter turn/stream handling, and automatic continuation runs—areas where races or wedged closes could leave stale CLI processes or mis-ordered wake vs user turns.
Overview
Fixes idle Claude provider session release deadlocking when the SDK message stream is interrupted mid-read, and adds orchestration for Claude background-task wake turns so buffered CLI output is ingested without orphaning processes or losing work.
Session close:
claudeQueryMessagesstreams via theQueryobject (not the rawsdkMessagesgenerator) soQuery.return()runs transport cleanup before an in-flight read. Provider session manager time-boxes scope close (30s), persists released events on timeout, and defers idle release whilehasPendingBackgroundWorkis true (capped bymaxIdlePinMs), with generation guards against stale idle decisions.Wake turns: The Claude adapter buffers SDK messages when no turn is active, tracks background
local_bashtasks, emits oneProviderContinuationRequestper wake, and drains the buffer on provider-authored continuation turns without re-offering the CLI. A new ProviderContinuationService worker dispatches internalmessage.dispatchruns (queue_after_active). Native session resume now considers persistedproviderTurnOrdinal > 1after idle release, not only in-memoryopenedNativeThreads.Reviewed by Cursor Bugbot for commit 84245db. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix Claude session release hanging on idle CLI reads by introducing background wake turn buffering
claudeQueryMessagesin ClaudeAdapterV2.ts to iterate the Claude Query runtime (not the raw generator), ensuringQuery.return()runs transport cleanup before the in-flight read.ProviderContinuationRequestis issued per wake event, and a provider-authored continuation turn drains buffered messages without re-prompting the CLI.ThreadManagementServiceto trigger continuation runs.hasPendingBackgroundWorkis true, capped bymaxIdlePinMsto prevent indefinite pinning; idle generation guards prevent races with newly busy sessions.Macroscope summarized 84245db.